[Data] Introduce RenameExpr to separate rename semantics from AliasExpr#60804
Open
slfan1989 wants to merge 3 commits intoray-project:masterfrom
Open
[Data] Introduce RenameExpr to separate rename semantics from AliasExpr#60804slfan1989 wants to merge 3 commits intoray-project:masterfrom
slfan1989 wants to merge 3 commits intoray-project:masterfrom
Conversation
This PR introduces a dedicated RenameExpr type to replace the _is_rename
flag in AliasExpr, providing clearer semantics and better type safety for
column renaming operations.
Changes:
- Add RenameExpr class for explicit column renaming semantics
- Remove _is_rename flag from AliasExpr (now purely for computed columns)
- Update all visitors to handle RenameExpr (9 visitor implementations)
- Update logical optimization rules for clearer rename vs alias handling
- Add comprehensive tests for RenameExpr behavior
Behavioral changes:
- col("a")._rename("b") now creates RenameExpr instead of AliasExpr
- col("a").alias("b") remains AliasExpr (semantically correct for aliasing)
- Projection pushdown now correctly distinguishes rename from alias operations
This resolves the TODO in _ColumnSubstitutionVisitor and enables future
optimizations by making rename semantics explicit in the type system.
Signed-off-by: slfan1989 <slfan1989@apache.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated RenameExpr to separate column renaming from aliasing computed columns, which was previously handled by AliasExpr with an internal flag. This is a significant improvement for code clarity and type safety. The changes are well-executed, with updates to all necessary expression visitors and logical optimization rules. The addition of comprehensive tests for RenameExpr ensures the new functionality is robust. I have one suggestion to improve the consistency of expression substitution logic.
Comment on lines
285
to
290
| visited = self.visit(expr.expr) | ||
| if isinstance(visited, ColumnExpr): | ||
| return RenameExpr( | ||
| data_type=visited.data_type, expr=visited, _name=expr.name | ||
| ) | ||
| return AliasExpr(data_type=visited.data_type, expr=visited, _name=expr.name) |
Contributor
There was a problem hiding this comment.
For consistency with visit_alias, consider calling _unalias() on the visited expression. This would prevent creating nested aliases if the substitution itself results in an AliasExpr and would correctly preserve the RenameExpr type when possible.
For example, if col('a')._rename('b') is visited with a substitution {'a': col('c').alias('d')}:
- The current implementation produces a nested alias:
(col('c').alias('d')).alias('b'). - With
_unalias(), it would becomecol('c')._rename('b'), which seems more correct as the intermediate aliasdis being renamed tobanyway.
Suggested change
| visited = self.visit(expr.expr) | |
| if isinstance(visited, ColumnExpr): | |
| return RenameExpr( | |
| data_type=visited.data_type, expr=visited, _name=expr.name | |
| ) | |
| return AliasExpr(data_type=visited.data_type, expr=visited, _name=expr.name) | |
| visited = self.visit(expr.expr)._unalias() | |
| if isinstance(visited, ColumnExpr): | |
| return RenameExpr( | |
| data_type=visited.data_type, expr=visited, _name=expr.name | |
| ) | |
| return AliasExpr(data_type=visited.data_type, expr=visited, _name=expr.name) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
python/ray/data/_internal/planner/plan_expression/expression_visitors.py
Show resolved
Hide resolved
This PR introduces a dedicated RenameExpr type to replace the _is_rename
flag in AliasExpr, providing clearer semantics and better type safety for
column renaming operations.
Changes:
- Add RenameExpr class for explicit column renaming semantics
- Remove _is_rename flag from AliasExpr (now purely for computed columns)
- Update all visitors to handle RenameExpr (9 visitor implementations)
- Update logical optimization rules for clearer rename vs alias handling
- Add comprehensive tests for RenameExpr behavior
Behavioral changes:
- col("a")._rename("b") now creates RenameExpr instead of AliasExpr
- col("a").alias("b") remains AliasExpr (semantically correct for aliasing)
- Projection pushdown now correctly distinguishes rename from alias operations
This resolves the TODO in _ColumnSubstitutionVisitor and enables future
optimizations by making rename semantics explicit in the type system.
Signed-off-by: slfan1989 <slfan1989@apache.org>
This PR introduces a dedicated RenameExpr type to replace the _is_rename
flag in AliasExpr, providing clearer semantics and better type safety for
column renaming operations.
Changes:
- Add RenameExpr class for explicit column renaming semantics
- Remove _is_rename flag from AliasExpr (now purely for computed columns)
- Update all visitors to handle RenameExpr (9 visitor implementations)
- Update logical optimization rules for clearer rename vs alias handling
- Add comprehensive tests for RenameExpr behavior
Behavioral changes:
- col("a")._rename("b") now creates RenameExpr instead of AliasExpr
- col("a").alias("b") remains AliasExpr (semantically correct for aliasing)
- Projection pushdown now correctly distinguishes rename from alias operations
This resolves the TODO in _ColumnSubstitutionVisitor and enables future
optimizations by making rename semantics explicit in the type system.
Signed-off-by: slfan1989 <slfan1989@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a dedicated
RenameExprtype to replace the_is_renameflag inAliasExpr, providing clearer semantics and better type safety for column renaming operations in Ray Data expressions.Key Changes:
RenameExprclass for explicit column renaming semantics_is_renameflag fromAliasExpr(now purely for computed columns/aliasing)RenameExprRenameExprbehavior and structural equalityBehavioral Changes:
col("a")._rename("b")now createsRenameExprinstead ofAliasExprcol("a").alias("b")remainsAliasExpr(semantically correct for aliasing computed expressions)Benefits:
_ColumnSubstitutionVisitorRelated issues
N/A
Additional information
This refactoring maintains backward compatibility while improving the internal representation. All existing tests pass, and new tests validate the
RenameExprbehavior including structural equality, evaluation equivalence, and proper handling during column substitution.